Encode Node information in separate types in aggregation rules #137

eapolinario · 2020-08-28T23:26:02Z

This PR redefines how the node information is used in aggregation rules.

So far we have relied on the presence of the NodeFieldType to decide which field from the envoy Node object we used for both (1) matching, and (2) fragment generation. In this PR we define separate types for each one of the two cases (i.e. (1) and (2)).

As we spoke offline, we opted for not using the envoy protos, instead, we're going to maintain a mapping between the envoy objects and the objects needed to perform (1) and (2) so as to isolate ourselves from other migration issues. Concretely, the envoy v2/v3 Locality object has its properties duplicated in our copy of the LocalityMatch and LocalityResultAction.

The idea behind this refactoring is to allow for better extensibility of both the matching and fragment generation phases. In summary, the process of adding a new type of aggregation rule object consists of a 2 step process:

Add a pair of types to do (1) and (2) to the aggregation rules proto.
Handle the newly added types in the process the types added in the above step in the mapper (including unit tests).

A followup PR is going to follow the model proposed in this PR to add support for node metadata.

Signed-off-by: eapolinario <[email protected]>

…node-in-type Signed-off-by: eapolinario <[email protected]>

Signed-off-by: eapolinario <[email protected]>

samrabelachew

lgtm

jyotimahapatra

The structure makes the yaml much readable. I've some concerns about using the corev2 protos outside of transport package. Can we isolate the usage?

jyotimahapatra · 2020-09-11T19:29:19Z

example/config-files/aggregation-rules.yaml

@@ -9,8 +9,7 @@ fragments:
              - "type.googleapis.com/envoy.api.v2.RouteConfiguration"
        result:
          request_node_fragment:
-            field: 1
-            action:
+            cluster_action:


i like how its a named action, instead of field: 1

internal/app/mapper/mapper.go

Signed-off-by: eapolinario <[email protected]>

eapolinario · 2020-09-11T21:11:01Z

internal/app/mapper/mapper.go

-	default:
-		return false, fmt.Errorf("RequestNodeMatch does not have a valid NodeFieldType")
+
+	// By construction only one of these is set at any point in time, so checking one by one


@jyotimahapatra , do you know if there's a way to preserve the switch-like functionality here? From a correctness perspective my comment is true, albeit less performant than the old code.

i think we don't have a way to deal with the oneof fields using switch.

Probably compareString(idMatch, req.GetNodeID()) || compareString(clusterMatch, req.GetCluster()) || compareLocality(localityMatch, req.GetLocality()) and deal with nilness inside compareString. But compareString returns 2 values 😞

relying on short-circuiting is not better in terms of perf than what I'm proposing here.

i think what we have here works.

jyotimahapatra

lgtm and few questions

jyotimahapatra · 2020-09-11T22:54:35Z

internal/app/mapper/mapper.go

-	default:
-		return false, fmt.Errorf("RequestNodeMatch does not have a valid NodeFieldType")
+
+	// By construction only one of these is set at any point in time, so checking one by one


i think we don't have a way to deal with the oneof fields using switch.

jyotimahapatra · 2020-09-11T22:56:56Z

internal/app/mapper/mapper.go

-	default:
-		return false, fmt.Errorf("RequestNodeMatch does not have a valid NodeFieldType")
+
+	// By construction only one of these is set at any point in time, so checking one by one


Probably compareString(idMatch, req.GetNodeID()) || compareString(clusterMatch, req.GetCluster()) || compareLocality(localityMatch, req.GetLocality()) and deal with nilness inside compareString. But compareString returns 2 values 😞

jyotimahapatra · 2020-09-11T22:59:54Z

internal/app/mapper/mapper.go

+	}
+
+	// N.B.: join matches using "|" to indicate they all came from the locality object.
+	return strings.Join(matches, "|"), nil


not for this PR. We might want to document it.

wondering if a mapper_test should cover this?

the tests I added in mapper_test all rely on behavior, e.g.

xds-relay/internal/app/mapper/mapper_test.go

Line 319 in 3155439

fmt.Sprintf("%s|%s|%s", noderegion, nodezone, nodesubzone),

.

thanks for showing this

jyotimahapatra · 2020-09-11T23:24:48Z

internal/app/mapper/mapper_test.go

-		},
-	},
-	{
-		Description: "RequestNodeMatch with regex match request node zone mismatch",


I think we removed this tests by mistake? Lets add them back

I think this is the list that needs adding back.

RequestNodeMatch with node region regex does not match RequestNodeMatch with node zone regex does not match RequestNodeMatch with node subzone regex does not match RequestNodeMatch with regex match request node region mismatch RequestNodeMatch with regex match request node zone mismatch empty node region in request predicate empty node zone in request predicate empty node subzone in request predicate non matching field type

…proxy#137) * Add NodeStringMatch proto definitions Signed-off-by: eapolinario <[email protected]> * Expose Locality in RequestV2 Signed-off-by: eapolinario <[email protected]> * Replace uses of node field type in isNodeMatch Signed-off-by: eapolinario <[email protected]> * fix mapper_test tests cases related to node cluster and node id. Signed-off-by: eapolinario <[email protected]> * lint Signed-off-by: eapolinario <[email protected]> * Use the new protos in tests Signed-off-by: eapolinario <[email protected]> * bump dependencies. Signed-off-by: eapolinario <[email protected]> * more tests. Signed-off-by: eapolinario <[email protected]> * Fix remaining unit tests. Signed-off-by: eapolinario <[email protected]> * remove TODO from proto Signed-off-by: eapolinario <[email protected]> * Remove getResultRequestNodeFragment Signed-off-by: eapolinario <[email protected]> * Fix yaml_proto tests Signed-off-by: eapolinario <[email protected]> * Add NodeLocality to RequestNodeFragment Signed-off-by: eapolinario <[email protected]> * Edit getResultFromRequestNodePredicate Signed-off-by: eapolinario <[email protected]> * fix integration tests Signed-off-by: eapolinario <[email protected]> * delete unused getNodeValue function Signed-off-by: eapolinario <[email protected]> * Fix e2e test. Signed-off-by: eapolinario <[email protected]> * uncomment Locality tests Signed-off-by: eapolinario <[email protected]> * Define LocalityResultAction Signed-off-by: eapolinario <[email protected]> * fix example aggregation rules file. Signed-off-by: eapolinario <[email protected]> * s/NodeStringMatch/StringMatch/g Signed-off-by: eapolinario <[email protected]> * s/NodeLocalityMatch/LocalityMatch + use StringMatch Signed-off-by: eapolinario <[email protected]> * lint Signed-off-by: eapolinario <[email protected]> * remove NodeFieldType enum. Signed-off-by: eapolinario <[email protected]> * more linting Signed-off-by: eapolinario <[email protected]> * remove mentions to enum in test file. Signed-off-by: eapolinario <[email protected]> * Remove mentions to corev2.Locality and use our own Locality object Signed-off-by: eapolinario <[email protected]> * remove TODO in mapper. Signed-off-by: eapolinario <[email protected]> * remove individual Locality properties. Signed-off-by: eapolinario <[email protected]> Signed-off-by: Samra Belachew <[email protected]>

This PR adds a matcher for node metadata fields. This is for #125. Similarly to #137, we encode the node metadata information required in the aggregation rules in a pair of protos: (1) NodeMetadataMatch, and (2) NodeMetadataAction, the former related to the matching process, whereas the latter has to do with the formation of the fragment. Envoy's node metadata is an opaque struct of type google.protobuf.Struct, which basically represents a dictionary of strings to Value. This PR adds support for 3 of the 6 values: string, bool, and a nested struct of Value. The support for these other types is going to be added in separate PRs.

eapolinario added 27 commits August 28, 2020 11:28

Add NodeStringMatch proto definitions

58e32b4

Signed-off-by: eapolinario <[email protected]>

Expose Locality in RequestV2

79b2904

Signed-off-by: eapolinario <[email protected]>

Replace uses of node field type in isNodeMatch

4292fc2

Signed-off-by: eapolinario <[email protected]>

fix mapper_test tests cases related to node cluster and node id.

cd3e7f0

Signed-off-by: eapolinario <[email protected]>

lint

683e402

Signed-off-by: eapolinario <[email protected]>

Use the new protos in tests

6ac0898

Signed-off-by: eapolinario <[email protected]>

Merge branch 'master' of github.com:envoyproxy/xds-relay into encode-…

ee07b37

…node-in-type Signed-off-by: eapolinario <[email protected]>

bump dependencies.

1d1bd1e

Signed-off-by: eapolinario <[email protected]>

more tests.

3530926

Signed-off-by: eapolinario <[email protected]>

Fix remaining unit tests.

eb11170

Signed-off-by: eapolinario <[email protected]>

remove TODO from proto

f414e06

Signed-off-by: eapolinario <[email protected]>

Remove getResultRequestNodeFragment

911df69

Signed-off-by: eapolinario <[email protected]>

Fix yaml_proto tests

dd313a2

Signed-off-by: eapolinario <[email protected]>

Add NodeLocality to RequestNodeFragment

d3b301f

Signed-off-by: eapolinario <[email protected]>

Edit getResultFromRequestNodePredicate

9610dc8

Signed-off-by: eapolinario <[email protected]>

fix integration tests

a7230f3

Signed-off-by: eapolinario <[email protected]>

delete unused getNodeValue function

92eacd9

Signed-off-by: eapolinario <[email protected]>

Fix e2e test.

a9e7629

Signed-off-by: eapolinario <[email protected]>

uncomment Locality tests

658409b

Signed-off-by: eapolinario <[email protected]>

Define LocalityResultAction

2d0416a

Signed-off-by: eapolinario <[email protected]>

fix example aggregation rules file.

805c0f4

Signed-off-by: eapolinario <[email protected]>

s/NodeStringMatch/StringMatch/g

509de35

Signed-off-by: eapolinario <[email protected]>

s/NodeLocalityMatch/LocalityMatch + use StringMatch

076a3f1

Signed-off-by: eapolinario <[email protected]>

lint

175d567

Signed-off-by: eapolinario <[email protected]>

remove NodeFieldType enum.

d09afb7

Signed-off-by: eapolinario <[email protected]>

more linting

56a1b7c

Signed-off-by: eapolinario <[email protected]>

remove mentions to enum in test file.

12ef5ce

Signed-off-by: eapolinario <[email protected]>

eapolinario changed the title ~~WIP - Encode Node information in separate types in aggregation rules~~ Encode Node information in separate types in aggregation rules Sep 1, 2020

eapolinario requested review from jyotimahapatra and jessicayuen September 1, 2020 08:53

eapolinario requested a review from samrabelachew September 3, 2020 18:14

samrabelachew previously approved these changes Sep 11, 2020

View reviewed changes

jyotimahapatra reviewed Sep 11, 2020

View reviewed changes

eapolinario added 2 commits September 11, 2020 13:38

merge conflicts

1eda1a1

Signed-off-by: eapolinario <[email protected]>

Remove mentions to corev2.Locality and use our own Locality object

9dead19

Signed-off-by: eapolinario <[email protected]>

eapolinario dismissed samrabelachew’s stale review via 9dead19 September 11, 2020 20:56

eapolinario added 2 commits September 11, 2020 14:01

remove TODO in mapper.

85ba9aa

Signed-off-by: eapolinario <[email protected]>

remove individual Locality properties.

3155439

Signed-off-by: eapolinario <[email protected]>

eapolinario commented Sep 11, 2020

View reviewed changes

jyotimahapatra reviewed Sep 11, 2020

View reviewed changes

jyotimahapatra approved these changes Sep 11, 2020

View reviewed changes

eapolinario merged commit f875109 into envoyproxy:master Sep 11, 2020

jyotimahapatra reviewed Sep 11, 2020

View reviewed changes

eapolinario mentioned this pull request Sep 12, 2020

Add tests back #143

Merged

eapolinario mentioned this pull request Sep 28, 2020

Node metadata matcher #154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode Node information in separate types in aggregation rules #137

Encode Node information in separate types in aggregation rules #137

eapolinario commented Aug 28, 2020 •

edited

Loading

samrabelachew left a comment

jyotimahapatra left a comment

jyotimahapatra Sep 11, 2020

eapolinario Sep 11, 2020

jyotimahapatra Sep 11, 2020

jyotimahapatra Sep 11, 2020

eapolinario Sep 11, 2020

jyotimahapatra Sep 11, 2020

jyotimahapatra left a comment

jyotimahapatra Sep 11, 2020

jyotimahapatra Sep 11, 2020

jyotimahapatra Sep 11, 2020

jyotimahapatra Sep 11, 2020

eapolinario Sep 11, 2020

jyotimahapatra Sep 11, 2020

jyotimahapatra Sep 11, 2020

jyotimahapatra Sep 11, 2020 •

edited

Loading

Encode Node information in separate types in aggregation rules #137

Encode Node information in separate types in aggregation rules #137

Conversation

eapolinario commented Aug 28, 2020 • edited Loading

samrabelachew left a comment

Choose a reason for hiding this comment

jyotimahapatra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyotimahapatra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyotimahapatra Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

eapolinario commented Aug 28, 2020 •

edited

Loading

jyotimahapatra Sep 11, 2020 •

edited

Loading